Updating added types for ReadableStreamReadDoneResult#1676
Updating added types for ReadableStreamReadDoneResult#1676kraenhansen wants to merge 4 commits intomicrosoft:mainfrom
ReadableStreamReadDoneResult#1676Conversation
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
|
Makes sense, bu you need to build and commit the result. |
|
Actually byte streams indeed return the value, btw. Perhaps split the type? https://streams.spec.whatwg.org/#byob-reader-internal-slots |
|
Hi @MattiasBuelens, would you be satisfied with this? Also it would be good to have some unit tests here. |
|
I've updated the code to separate the
I've also added The two new types reuse
The PR is currently failing tests: I can't seem to get the specialised |
inputfiles/addedTypes.jsonc
Outdated
| "value": { | ||
| "name": "value", | ||
| "overrideType": "T" | ||
| "overrideType": "T", |
There was a problem hiding this comment.
ReadableStreamBYOBReadDoneResult.value should be T | undefined, as per spec:
If the reader is canceled, the promise will be fulfilled with an object of the form
{ value: undefined, done: true }. In this case, the backing memory region of view is discarded and not returned to the caller.
See also: Deno and web-streams-polyfill.
| "name": "value", | ||
| "overrideType": "T" | ||
| "overrideType": "T", | ||
| "required": true |
There was a problem hiding this comment.
If we make value: T | undefined, should we also make it optional? Or does that not work well together with exactOptionalPropertyTypes?
There was a problem hiding this comment.
I don't think so, the spec is pretty clear that the value property would exist: https://streams.spec.whatwg.org/#default-reader-read
There was a problem hiding this comment.
That's for default readers. For BYOB readers, the spec is at https://streams.spec.whatwg.org/#byob-reader-read.
But still, that doesn't explain where chunk is coming from. When a stream is cancelled while it has a BYOB reader attached, ReadableStreamCancel resolves all pending read(view) promises with { done: true, value: undefined }:
- If reader is not undefined and reader implements ReadableStreamBYOBReader,
6.3. For each readIntoRequest of readIntoRequests,
6.3.1. Perform readIntoRequest’s close steps, given undefined.
There was a problem hiding this comment.
You're right. Reading the spec you've linked to, it seems pretty explicit (list item 9) that the value property will be in the object returned from read, even from a ReadableStreamBYOBReader.
The Streams types have |
I found the issue 👍 |
3 participants
This fixes #1675.